Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support BREAKING CHANGE #168

Merged
merged 8 commits into from
Feb 17, 2024
Merged

feat: support BREAKING CHANGE #168

merged 8 commits into from
Feb 17, 2024

Conversation

hyoban
Copy link
Contributor

@hyoban hyoban commented Feb 15, 2024

PR Checklist

Overview

continue #140
related conventional-changelog/conventional-changelog#648

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A great start, thanks for sending this! 🙌

Requesting a bit of touchups around testing and types, but nothing major. The implementation looks solid. Nice find on the breakingHeaderPattern!

README.md Outdated Show resolved Hide resolved
src/getCommitMeaning.test.ts Show resolved Hide resolved
src/getCommitMeaning.test.ts Show resolved Hide resolved
Comment on lines 12 to 15
const { notes, type } = conventionalCommitsParser.sync(message, {
// @ts-expect-error - options from https://github.com/conventional-changelog/conventional-changelog/issues/648#issuecomment-704867077
breakingHeaderPattern: /^(\w*)(?:\((.*)\))?!: (.*)$/,
});
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Refactor] No need for a // @ts-expect-error when an assertion can do:

Suggested change
const { notes, type } = conventionalCommitsParser.sync(message, {
// @ts-expect-error - options from https://github.com/conventional-changelog/conventional-changelog/issues/648#issuecomment-704867077
breakingHeaderPattern: /^(\w*)(?:\((.*)\))?!: (.*)$/,
});
// options from https://github.com/conventional-changelog/conventional-changelog/issues/648#issuecomment-704867077
const { notes, type } = conventionalCommitsParser.sync(message, {
breakingHeaderPattern: /^(\w*)(?:\((.*)\))?!: (.*)$/,
} as object);

This is actually a great opportunity to clean up the DefinitelyTyped types for conventional-commits-parser. Is that something you have time for too? No worries if not, I will if you don't. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to deal with it. It’s always exciting to have the opportunity to work on an open source project.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author Needs an action taken by the original poster label Feb 16, 2024
hyoban and others added 3 commits February 17, 2024 08:26
@github-actions github-actions bot removed the status: waiting for author Needs an action taken by the original poster label Feb 17, 2024
Copy link

codecov bot commented Feb 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c600bb3) 63.55% compared to head (9a3bd69) 65.78%.
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
+ Coverage   63.55%   65.78%   +2.23%     
==========================================
  Files           6        6              
  Lines         107      114       +7     
  Branches       12       15       +3     
==========================================
+ Hits           68       75       +7     
  Misses         39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this looks great - thanks! 🙌

@JoshuaKGoldberg
Copy link
Owner

Note that the compliance PR action is failing because this PR is sent from https://github.com/hyoban/should-semantic-release/tree/main, rather than a separate branch. https://www.joshuakgoldberg.com/blog/contributing-to-a-create-typescript-app-repository-faqs/#how-do-i-reset-my-main-back-to-the-upstreams has some tips for dealing with that. Not a blocker, just a heads up. 🙂

@JoshuaKGoldberg JoshuaKGoldberg merged commit a642e10 into JoshuaKGoldberg:main Feb 17, 2024
13 of 14 checks passed
@JoshuaKGoldberg
Copy link
Owner

@all-contributors please add @hyoban for code.

🤖 Beep boop! This comment was added automatically by all-contributors-auto-action.
Not all contributions can be detected from Git & GitHub alone. Please comment any missing contribution types this bot missed.
...and of course, thank you for contributing! 💙

Copy link
Contributor

@JoshuaKGoldberg

I've put up a pull request to add @hyoban! 🎉

I couldn't determine any contributions to add, did you specify any contributions?
Please make sure to use valid contribution names.

@hyoban
Copy link
Contributor Author

hyoban commented Feb 17, 2024

Thanks, I'll be careful next time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Return true for BREAKING CHANGE?
2 participants